Skip to content

[IOTDB-1597] Finish DIFF encoding algorithm which is reserved in enum before #1890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
5Reeson wants to merge 11 commits into from
Closed

[IOTDB-1597] Finish DIFF encoding algorithm which is reserved in enum before #1890

5Reeson wants to merge 11 commits into from

Conversation

@5Reeson
Copy link

@5Reeson 5Reeson commented Oct 28, 2020 •
edited by jixuan1989
Loading

In the enum class: TSEncoding, there's DIFF encoding, so when user want to choose a type of encoding algorithm(or input the wrong encoding type name), DIFF encoding is in the output hint and is one of IoTDB's recommending encoding type,
but in IoTDB, DIFF encoding has not been added before, which I think may cause some problems.
I think it's necessary to finish it.

Both DIFF and TS_2DIFF encoding are based on delta encoding algorithm,
so I finish DIFF using similar interfaces and structures like TS_2DIFF to make it easier to understand.

HTHou
HTHou requested changes Oct 29, 2020
View reviewed changes
jixuan1989
jixuan1989 reviewed Nov 1, 2020
View reviewed changes
@jixuan1989
Copy link
Member

jixuan1989 commented Nov 3, 2020

please apply this patch to solve the conflicts.
git apply solve_conflict.txt
solve_conflict.txt

@jixuan1989
Copy link
Member

jixuan1989 commented Nov 3, 2020

  1. there are too many duplications between DIFFEncoder and DeltaBinaryEncoder. Better to inherit DeltaBinaryEncoder, rather than copy codes.

  2. What is the benefit to add such a method? Trying to give some experimental results to show when should we use it...

@5Reeson
Copy link
Author

5Reeson commented Nov 3, 2020

  1. there are too many duplications between DIFFEncoder and DeltaBinaryEncoder. Better to inherit DeltaBinaryEncoder, rather than copy codes.
  2. What is the benefit to add such a method? Trying to give some experimental results to show when should we use it...

I have made some experiment abouot it(https://cwiki.apache.org/confluence/display/IOTDB/DIFF+encoding%27s+perfomance).
As I said in email-list, this method's performance is not better than TS_2DIFF (included in 'DeltaBinaryEncoder'), only when the data set is in best circumstances, the new method can save very little room (about 4 bytes, in head)

I add it beacuse DIFF is a reserved method, it's one of the recommending method when user want to choose a encoding method in IoTDB, but it's not finished before. It's in the system prompt, but user can't chooose it
system_prompt

@JackieTien97
Copy link
Contributor

JackieTien97 commented Dec 14, 2020

Hi, please resolve the conflicts

@HTHou HTHou added the Module - TsFile data file format label Dec 21, 2020
HTHou
HTHou reviewed Aug 31, 2021
View reviewed changes
@HTHou HTHou changed the title Finish DIFF encoding algorithm which is reserved in enum before [IOTDB-1597] Finish DIFF encoding algorithm which is reserved in enum before Aug 31, 2021
HTHou
HTHou approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@HTHou HTHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better now.

@coveralls
Copy link

coveralls commented Aug 31, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 67.374% when pulling d9881b2 on 5Reasons:addDiff into fb18357 on apache:master.

@HTHou HTHou closed this Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jixuan1989 jixuan1989 jixuan1989 left review comments

@kr11 kr11 kr11 left review comments

@HTHou HTHou HTHou approved these changes

Assignees

No one assigned

Labels

Module - TsFile data file format

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@5Reeson @jixuan1989 @JackieTien97 @coveralls @kr11 @HTHou